Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: implements aws oidc backendSecurityPolicy API #306

Merged
merged 86 commits into from
Feb 18, 2025
Merged

feat: implements aws oidc backendSecurityPolicy API #306

merged 86 commits into from
Feb 18, 2025

Conversation

aabchoo
Copy link
Contributor

@aabchoo aabchoo commented Feb 6, 2025 •
edited by yuzisun
Loading

Commit Message

The PR implements the backendSecurityPolicy API for AWS's OIDC credentials.

Related Issues/PRs (if applicable)

Special notes for reviewers (if applicable)

I've tested the implementation with our SSO (oauth2), and was able to query bedrock.

@aabchoo aabchoo added feature go Pull requests that update Go code labels Feb 6, 2025
@mathetake
Copy link
Member

I think you are missing two things (not serious):

otherwise, looking good

@aabchoo
Copy link
Contributor Author

aabchoo commented Feb 6, 2025

One scenario I realize that needs to be address is:

  1. OIDC credentials or AWS specific fields are updated
  2. Either or both values are cached/not expired.

We may need to cache the oidc credentials and aws specific fields to compare. Else users will have to wait til expiration time (ie 1 hour for AWS or however long for OIDC)

yuzisun
yuzisun reviewed Feb 7, 2025
View reviewed changes
internal/controller/oauth/oidc_provider.go Outdated
}

// OIDCMetadata represents the OpenID Connect provider metadata
type OIDCMetadata struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use this type from go-oidc
https://github.com/coreos/go-oidc/blob/v3/oidc/oidc.go#L181

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the field ScopesSupported - will see how we can include it

yuzisun
yuzisun reviewed Feb 9, 2025
View reviewed changes
yuzisun
yuzisun reviewed Feb 9, 2025
View reviewed changes
@mathetake mathetake added this to the v.0.1.0 milestone Feb 12, 2025
@yuzisun yuzisun force-pushed the aaron/oidc-revisited branch from 10553c4 to 1eab805 Compare February 13, 2025 02:44
@aabchoo aabchoo marked this pull request as ready for review February 13, 2025 16:10
@aabchoo aabchoo requested a review from a team as a code owner February 13, 2025 16:10
@aabchoo
Copy link
Contributor Author

aabchoo commented Feb 13, 2025

Thought I had enough test coverage - will bump that up

mathetake
mathetake reviewed Feb 13, 2025
View reviewed changes
Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job! i am still half way through it but left some comments!

mathetake
mathetake reviewed Feb 13, 2025
View reviewed changes
mathetake
mathetake reviewed Feb 13, 2025
View reviewed changes
mathetake
mathetake reviewed Feb 13, 2025
View reviewed changes
internal/controller/rotators/common.go
secret := &corev1.Secret{}
if err := k8sClient.Get(ctx, client.ObjectKey{
Namespace: namespace,
Name: name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a prefix like ai-eg-bsp-?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially it was used to create any secrets but I've modified it to be bsp specific. Added a prefix. Kept LookUpSecret without prefix because it's being used to get more than bsp secrets (primarily in testing).

mathetake
mathetake reviewed Feb 13, 2025
View reviewed changes
mathetake
mathetake reviewed Feb 13, 2025
View reviewed changes
aabchoo and others added 12 commits February 16, 2025 16:04
@aabchoo @yuzisun
getClientSeret more descriptive error
6720aee 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
support only one credential profile per file
df12726 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
region always exists
db334d4 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
use backendSecurityPolicyKey func to reduce replication
98f6001 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
stsOp -> stsClient
35f3703 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
stop storing ctx
0a2dcd0 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
split credential renewal from reconcile
987b355 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
fix tests
17e4123 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo @yuzisun
sync oidc between providers
ab98e14 
Signed-off-by: Aaron Choo <[email protected]>
@yuzisun
Remove the skipOIDC flag
f592bc2 
Signed-off-by: Dan Sun <[email protected]>
@yuzisun
Don't export NewClientCredentialsProvider
682293f 
Signed-off-by: Dan Sun <[email protected]>
@yuzisun
Don't export NewClientCredentialsProvider
dc5f31c 
Signed-off-by: Dan Sun <[email protected]>
@yuzisun yuzisun force-pushed the aaron/oidc-revisited branch from e963652 to dc5f31c Compare February 16, 2025 21:04
@yuzisun yuzisun requested review from mathetake and yuzisun February 17, 2025 15:45
@yuzisun
Copy link
Contributor

yuzisun commented Feb 17, 2025

@mathetake this is ready for final review

mathetake
mathetake reviewed Feb 18, 2025
View reviewed changes
Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments but good to go!

@aabchoo
preallocate map
ce36ffb 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo
update mockSTSOperations to lower
0727c2b 
Signed-off-by: Aaron Choo <[email protected]>
@aabchoo
update comment
55c243c 
Signed-off-by: Aaron Choo <[email protected]>
mathetake
mathetake approved these changes Feb 18, 2025
View reviewed changes
Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@mathetake mathetake merged commit d602297 into main Feb 18, 2025
17 checks passed
@mathetake mathetake deleted the aaron/oidc-revisited branch February 18, 2025 16:37
daixiang0 pushed a commit to daixiang0/ai-gateway that referenced this pull request Feb 19, 2025
@aabchoo @yuzisun
@mathetake @daixiang0
feat: implements aws oidc backendSecurityPolicy API (envoyproxy#306)
17d00f0 
**Commit Message**

The PR implements the backendSecurityPolicy API for AWS's OIDC
credentials.

**Related Issues/PRs (if applicable)**


**Special notes for reviewers (if applicable)**

I've tested the implementation with our SSO (oauth2), and was able to
query bedrock.

---------

Signed-off-by: Aaron Choo <[email protected]>
Co-authored-by: Dan Sun <[email protected]>
Co-authored-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Loong <[email protected]>
@xiaolin593 xiaolin593 mentioned this pull request Feb 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathetake mathetake mathetake approved these changes

@yuzisun yuzisun Awaiting requested review from yuzisun

Assignees

@mathetake mathetake

Labels
feature go Pull requests that update Go code
Projects
None yet
Milestone
v.0.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@aabchoo @mathetake @yuzisun